Open in StackBlitz

npm i https://pkg.pr.new/knip@1849

npm i https://pkg.pr.new/@knip/language-server@1849

npm i https://pkg.pr.new/@knip/mcp@1849

commit: 9353488

Thanks for the PR. Overall I feel like it's a bit much for this particular repo. We're not using pull_request_target and have zero secrets, so I wonder what's the real benefit here? No publish/release flow (at least not yet).

Not sure we want cancel-in-progress: true either: we should see if something fails in e.g Windows and Ubuntu (or only one of those), and the ecosystem repos (integration.yml) each have their own results we should see all failures for.

The no-cache: true is a bit overkill as it's scoped by branch?

The thing that we should definitely push though is the moved/scoped issues: write permission in integration.yml.

It might indeed be overkill here, but at the same time, it can't hurt nonetheless. It's also okay for me to not add it for now, only if we decide to create some release workflows where a prt trigger might be needed.

Not sure we want cancel-in-progress: true either: we should see if something fails in e.g Windows and Ubuntu (or only one of those), and the ecosystem repos (integration.yml) each have their own results we should see all failures for.

Just to clarify: the cancel-in-progress blocks have the effect of stopping running workflows when a new commit is pushed to the PR, because new workflows are started anyways with the new changes. So this does not mean that we will not see something failing. The workflows run exactly the same as before for the last commit. If you intend to always run workflows to the end, although there are already newer changes, then we should remove them. But normally, this block just saves unnecessary CI time, because older commit workflows are not that interesting anyways.

The no-cache: true is a bit overkill as it's scoped by branch?

I removed it 👍

General question: May I keep the newline changes or are do you want the dense version from before?

It might indeed be overkill here, but at the same time, it can't hurt nonetheless. It's also okay for me to not add it for now, only if we decide to create some release workflows where a prt trigger might be needed.

Let's add it when we need it. If there's no real use for Knip I think it's better to have less to look at in CI overview rather than more.

Not sure we want cancel-in-progress: true either: we should see if something fails in e.g Windows and Ubuntu (or only one of those), and the ecosystem repos (integration.yml) each have their own results we should see all failures for.

Just to clarify: the cancel-in-progress blocks have the effect of stopping running workflows when a new commit is pushed to the PR, because new workflows are started anyways with the new changes. So this does not mean that we will not see something failing. The workflows run exactly the same as before for the last commit. If you intend to always run workflows to the end, although there are already newer changes, then we should remove them. But normally, this block just saves unnecessary CI time, because older commit workflows are not that interesting anyways.

Thanks for clarifying this! TIL. I thought it was about in-flight runs for the same commit. In that case I think it's a nice feature.

The no-cache: true is a bit overkill as it's scoped by branch?

I removed it 👍

General question: May I keep the newline changes or are do you want the dense version from before?

Not a blocker and it doesn't bother me. But I'd say in general better not to change project style if it's just preference.

Removing zizmor from the PR would defeat its purpose so I'm going to close this one.